Skip to content

fix: preserve prepared ROUND and TRUNCATE value domains - #29508

Merged
XuPeng-SH merged 13 commits into
mainfrom
fix/29505-prepared-round-truncate-domain
Sep 30, 2026
Merged

XuPeng-SH merged 13 commits into
mainfrom
fix/29505-prepared-round-truncate-domain

Conversation

@XuPeng-SH

@XuPeng-SH XuPeng-SH commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • BUG

Which issue(s) this PR fixes:

Closes #29505
Closes #29509
Closes #29471
Closes #29506
Closes #29511
Closes #29512
Closes #29510
Closes #29514
Closes #29515
Closes #29516
Closes #29517

What this PR does / why we need it:

Design: reuse the existing prepared binding, expression-reset, and compile owners. The value-dependent proof uses the existing binding-state cache guard; no parallel cache or execution state machine was added.

Validation

Follow-up QA

The fix is scoped to comparisons where a unique DECIMAL value can be proven. Full sysbench/TPC-H/ClickBench workload performance remains subject to broader QA. Additional prepared expression and nonconstant peer shapes will continue to be challenged.

CI follow-up: assignment and JSON conversion boundaries

Latest repair commit: bc7c123ebd.

  • Preserve scalar JSON string payloads for implicit, comparison and assignment casts; explicit character CAST still serializes the JSON literal.
  • Preserve bare prepared assignment source types, including BIT string-byte semantics. Keep IGNORE conversion at the existing final assignment cast by applying one stateless policy at INSERT/VALUES/UPDATE/ODKU entry points; numeric functions and aggregates keep their independent consumer contexts.
  • Remove the obsolete BindContext.assignmentIgnore field and its propagation.
  • Reuse existing real COM_STMT fixtures for BIT float→text→float transitions and nine IGNORE source/operation combinations (INSERT SELECT, UPDATE scalar, upsert scalar × direct SELECT, derived SELECT, derived VALUES), plus arithmetic/ROUND/SUM controls. The new counterexamples failed before the repair and pass afterward.

Current local evidence:

  • Incremental gofmt/vet/lint over the complete changed Go package closure: PASS, 25 Go files / 5 packages, zero lint issues.
  • Full planner, function and frontend unit suites: PASS.
  • Full planner, function and frontend race unit suites: PASS.
  • Real prepared protocol regression tests plus TestPreparedSpecializedDomains, with race enabled: PASS.
  • Fifteen targeted multi-CN BVT files: PASS, including all added PR cases and the previously failing JSON/charset cases. The new JSON and charset cases also compare JDBC metadata; the other cases use CI's standard -n result comparison mode.
  • GPT-6.1-sol xhigh complete design/code review: no unresolved blocker.

Sequential TPCC 10-10 for bc7c123ebd versus GOOD 1575e70c3d completed: 4498.76 versus 4291.20 tpmC (+4.84%), zero errors, 2-minute warmup and 2-minute measurement on independent copies of the same initial snapshot. One pair does not establish a stable gain. That head later failed Ubuntu UT at issue29400's 2-second DROP deadline; Coverage separately exhausted retries on GitHub API HTTP 503.

Numeric predicate follow-up and CI test hardening

Latest repair: 811ab25f9c.

  • Reuse the existing expression visitor and unique DECIMAL peer proof for boolean descendants, IN/NOT IN lists, and BETWEEN endpoints. Unsafe, NULL, nonconstant and floating collision cases retain their existing comparison domain; executable peer expressions and explicit user CASTs remain intact.
  • Recognize a bound zero ROUND/TRUNCATE precision through existing configuration reads and exact integer proof. Reuse value-dependent cache protection; add no state or evaluator path.
  • Harden issue29400's DROP concurrency regression: open the peer connection before the proof and require A to remain alive at its actual fault barrier after B completes. The 10-second B deadline bounds the test; completion must precede barrier release. Keep the uncommitted transaction-tail check.

Validation on this source:

  • Full planner UT and race suites, frontend race suite, and real prepared protocol race test: PASS.
  • Updated issue29400 test: race, three consecutive runs: PASS. The original test also passed three local race runs; the remote log establishes a deadline failure but does not establish whether the operation was waiting for A's lock.
  • Incremental gofmt/vet/golangci-lint over the full PR closure: 26 Go files / 5 packages, zero issues.
  • All 15 related multi-CN BVT files: PASS. Updated issue29512 and issue29514 files also pass a second run on the same instance, with full EXPLAIN golden assertions for native Block Filter Cond. Reuse the existing tables and fixtures.
  • Independent QA: 22 result cases plus warning oracles, including NULL, unsafe mixed peers, positive/negative 2^53 collisions, malformed numeric warnings and recovery, precision 0/-1/NULL/0 reuse, and explicit CAST controls: PASS.
  • On 100,000 rows, 15 selective EXPLAIN ANALYZE probes scan one block / 8192 rows. The former OR/IN/BETWEEN and parameterized precision counterexamples scanned 13 blocks / 100,000 rows on bc7c123. On the candidate's same tables, full SUM and unsafe 0.104 controls still scan all 13 blocks, confirming the layout. Direct SQL EXECUTE IN/BETWEEN may retain a rendered DOUBLE cast while actual block pruning works.
  • Independent complete review using GPT-6.1-sol xhigh: PASS, zero remaining blockers, covering all 42 changed files and examining implementation, tests and golden increments separately.

Sequential TPCC 10-10 for 811ab25f9c versus GOOD 1575e70c3d completed: 4119.76 versus 3897.51 tpmC (+5.70%), 9297.80 versus 8718.62 tpmTOTAL, zero transaction errors on both, 2-minute warmup plus 2-minute measurement per version, candidate then GOOD with independent copies of the same initial snapshot. Other services, build and lint workloads were active on the shared machine. Sampled system I/O pressure was substantial; collection starts during the candidate measurement. These numbers are observations, not evidence of a stable gain or an isolated regression result. The deterministic block-count counterexamples above establish the targeted pruning improvement.

Fresh required CI is still running. The user requested not to wait for CI; local validation does not claim remote CI green or universal workload coverage.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of head 811ab25. I found two optimization coverage gaps below. The independent checks showed correct results; these cases miss block pruning, so they can be handled in a focused follow-up PR.

// normal comparison domain.
func preparedZeroPrecisionRoundParam(ctx context.Context, expr *Expr) *Expr {
fn := expr.GetF()
if fn == nil || fn.Func == nil || (fn.Func.GetObjName() != "round" && fn.Func.GetObjName() != "truncate") || len(fn.Args) != 2 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ROUND(expr) is also a valid one-argument form with default precision 0, but this proof only accepts two arguments. Consequently ROUND(?) and ROUND((SELECT ?)) skip the safe unique-domain rewrite. In the local 100k-row decimal-table check with bound value '99999.0', these forms read 13 blocks / 100,000 input rows, while the explicit ROUND(?,0) forms pruned to 1 block / 1 row. Please consider treating an omitted precision as literal 0 and covering both direct and scalar-subquery forms in a follow-up. This is a missed optimization; results remained correct.

Comment thread pkg/sql/plan/expr_opt.go
return nil
}
fn := current.GetF()
if fn == nil || fn.Func == nil || fn.Func.GetObjName() != "=" || len(fn.Args) != 2 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scalar-subquery rewrite is gated to =, so safe range predicates such as id >= ROUND((SELECT ?),0) keep the cast on the indexed column and miss block pruning. In the local check, that form read 13 blocks / 100,000 rows, while the direct-parameter id >= ROUND(?,0) form read 1 block. A follow-up can apply the same proof to supported comparison operators and bind the rewritten expression using the original operator name instead of hard-coding = here. Results remained correct in this case.

This branch was successfully deployed

1 active deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment